Skip to content

Conversation

@burtenshaw
Copy link
Collaborator

@burtenshaw burtenshaw commented Oct 27, 2025

This PR adapts scripts/prepare_hf_deployment.sh to take parameters env and namespace. This will allow community contributors to deploy their envs. Main changes from the original script are:

  • I simplified the logging a lot.
  • Add a push to hub step with the hf cli tool.
  • Added some convenience hub logic for things like authentication and namespace lookup.
  • Added a clean up step to rm the staging dir

To test this, you need to do:

bash scripts/deploy_to_hf.sh --env atari_env

out:

🙋 Using default namespace: burtenshaw. You can override the namespace with --hf-namespace
📁 Copied core and atari_env environment files to hf-staging/burtenshaw/atari_env
📝 Created README and web interface support for HF Space
🔑 Ensuring Hugging Face authentication...
No files have been modified since last commit. Skipping to prevent empty commit.
✅ Upload completed for https://huggingface.co/spaces/burtenshaw/atari_env-test

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 27, 2025
@jspisak jspisak requested a review from zkwentz October 27, 2025 17:06
@burtenshaw burtenshaw marked this pull request as ready for review October 27, 2025 18:55
@burtenshaw
Copy link
Collaborator Author

burtenshaw commented Oct 27, 2025

@zkwentz @jspisak ready for review 🙂

@pankit-eng
Copy link
Contributor

@burtenshaw - Can we try to reconcile .github/workflows/deploy_to_hf.yaml + existing script with this new script? I could be wrong but is it feasible to use this script and call it from deploy_to_hf.yaml and remove the previous bash script -prepare_hf_deployment.shlirkrk?

@zkwentz
Copy link
Contributor

zkwentz commented Oct 31, 2025

@burtenshaw thank you for this PR, +1 to @pankit-eng comment.

Additional deployment logic improvements should be addressed in a separate RFC focused on creating and maintaining OpenEnv environments (e.g., openenv init, openenv push).

While the example environments here are valuable, the real democratization happens through the hub. Here's what we need to build next:

Goal: Enable developers to push OpenEnv environments to the HuggingFace OpenEnv Hub

  • Support push functionality for OpenEnv-initialized (or converted) environments
  • Allow environments to be maintained in standalone repositories (GitHub, etc.)
  • Automatically publish to the HF OpenEnv Hub
  • Ensure discoverability by automatically adding to the OpenEnv Hub collection (in the future may become first-class primitive)

I'll provide full details in the upcoming RFC, but wanted to share this preview to get the conversation started.

@burtenshaw
Copy link
Collaborator Author

burtenshaw commented Oct 31, 2025

@burtenshaw - Can we try to reconcile .github/workflows/deploy_to_hf.yaml + existing script with this new script? I could be wrong but is it feasible to use this script and call it from deploy_to_hf.yaml and remove the previous bash script -prepare_hf_deployment.shlirkrk?

Yeah, that's totally possible. I deliberately separated them for ease. I'll upgrade this PR to keep the functionality of prepare_hf_deployment.sh and reconcile deploy_to_hf.yaml.

@burtenshaw
Copy link
Collaborator Author

Additional deployment logic improvements should be addressed in a separate RFC focused on creating and maintaining OpenEnv environments (e.g., openenv init, openenv push).

The rfc looks excellent, and this project style definition (openenv init, openenv push) will be very familiar to devs. In practical terms, I expect we can just reimplement much of this bash logic as a real cli.

@Darktex
Copy link
Contributor

Darktex commented Oct 31, 2025

I'm experimenting with having Claude Code do code reviews. What I normally do is run it first, and then review myself. For this particular PR, I'm not the best person to review it but I'm just copypasting what Claude is saying in case this helps.

Comprehensive Review: PR #102 - HF Deployment Script

Summary

PR #102 adds scripts/deploy_to_hf.sh, a new end-to-end deployment script for pushing OpenEnv environments to Hugging Face Spaces. This enables community contributors to deploy their
custom environments to their own HF namespaces.

Status: Good contribution, but needs architectural decision about relationship to existing deployment infrastructure.


The Core Question (@pankit-eng)

"Can we reconcile .github/workflows/deploy_to_hf.yaml + existing script with this new script?"

Short answer: Yes, and you should.


Current State Analysis

Existing Infrastructure:

  1. scripts/prepare_hf_deployment.sh (171 lines)
    - Prepares/stages files for deployment
    - Modifies Dockerfiles with specific base image SHA
    - Expects environment READMEs to have HF front matter
    - Does NOT push to HF
  2. .github/workflows/deploy-hf-env.yml (254 lines)
    - GitHub Actions CI/CD workflow
    - Calls prepare_hf_deployment.sh for staging
    - Uses git + HF_TOKEN to push to huggingface.co/spaces/openenv/
    - Hardcoded to 5 environments: echo_env, coding_env, chat_env, atari_env, openspiel_env

New Script (PR #102):

scripts/deploy_to_hf.sh (527 lines)

  • Complete end-to-end solution (preparation + push)
  • Uses hf CLI instead of git commands
  • Supports arbitrary environments (no hardcoded list)
  • Supports custom namespaces (not just "openenv")
  • Generates Dockerfiles and READMEs dynamically (doesn't require existing files)
  • Built for community contributors

Key Differences

Feature Old (prepare_hf_deployment.sh) New (deploy_to_hf.sh)
Scope Staging only Staging + Push
Dockerfiles Uses existing env Dockerfile Generates from templates
READMEs Requires HF front matter in env README Generates complete README
Push Method GitHub Actions with git hf CLI tool
Namespace Hardcoded to openenv Configurable (defaults to user)
Environment List Must be known environments Any environment under src/envs/
Use Case CI/CD (Meta team) Manual (community contributors)
Dependencies None (bash + git) Requires hf CLI

The Good ✅

  1. Enables Community Deployment

Before: Only Meta team could deploy to huggingface.co/spaces/openenv/
Now: Contributors can deploy to their own namespaces

  1. Better User Experience

Old way: Not possible for community contributors

(requires GitHub Actions + secrets)

New way: Simple one-liner

bash scripts/deploy_to_hf.sh --env my_custom_env

  1. More Flexible
  • Works with any environment (no hardcoded list)
  • Custom namespaces
  • Dry-run mode
  • Better error handling and validation
  1. Uses Modern HF Tooling

The hf CLI is the official recommended way to interact with HF Spaces now (vs git clones).

  1. Code Quality
  • Excellent error handling
  • Cross-platform sed compatibility
  • Clear help documentation
  • Proper parameter validation

The Issues ⚠️

  1. Duplication with Existing Infrastructure

You now have TWO ways to deploy to HF:

  • Old: prepare_hf_deployment.sh + GitHub Actions
  • New: deploy_to_hf.sh (standalone)

This creates maintenance burden. When you add a new environment (like textarena_env or wildfire_env), you need to update:

  • GitHub Actions workflow hardcoded list
  • Both Dockerfile generation logic
  • Both README generation logic
  1. Hardcoded Environment Knowledge

Lines 176-262 have hardcoded Dockerfile templates for specific envs:
case $env_name in
"echo_env") ... ;;
"coding_env") ... ;;
"chat_env") ... ;;
"atari_env") ... ;;
"openspiel_env") ... ;;
"textarena_env") ... ;;
esac

This means:

  • New environments need code changes to this script
  • Defeats the purpose of "works with any environment"
  • Custom community environments won't have proper Dockerfiles

Same issue in README generation (lines 296-473).

  1. Doesn't Reuse Existing Dockerfiles

The old prepare_hf_deployment.sh copies the environment's existing Dockerfile (src/envs/$ENV_NAME/server/Dockerfile), which is what's actually tested and maintained.

The new script generates Dockerfiles from templates, which could drift from reality.

  1. Missing Environments

The hardcoded list is already out of date:

  • Missing: git_env, sumo_rl_env, coding_env, python_codeact_env, dipg_safety_env
  • Has: Only 6 environments

Architectural Recommendations

Option A: Merge and Refactor (Recommended)

Create a unified approach:

  1. Keep deploy_to_hf.sh as the primary script (it's better)
  2. Refactor it to:
    - Use existing Dockerfiles from src/envs/$ENV/server/Dockerfile (like old script)
    - Support both standalone AND CI/CD use cases
    - Add --no-push flag (replaces old prepare_hf_deployment.sh)
  3. Update .github/workflows/deploy-hf-env.yml to:
    - Call deploy_to_hf.sh instead of prepare_hf_deployment.sh
    - Use environment variables for namespace/token
  4. Deprecate prepare_hf_deployment.sh

Benefits:

  • Single source of truth
  • Community AND CI/CD use same code path
  • Reuses actual tested Dockerfiles
  • Works with any environment automatically

Option B: Keep Both, Document Use Cases

Keep both scripts with clear separation:

  • prepare_hf_deployment.sh + workflow: For official OpenEnv deployments (CI/CD)
  • deploy_to_hf.sh: For community contributors (manual)

Benefits:

  • Less disruption to existing CI/CD
  • Clearer separation of concerns

Drawbacks:

  • Maintenance overhead (two codebases)
  • Duplication of Dockerfile/README logic

Specific Code Issues

  1. Authentication Logic Issue (lines 502-507)

if ! hf auth whoami | grep -qw "$HF_NAMESPACE"; then
echo "Error: Your account does not have access to the Hugging Face namespace '$HF_NAMESPACE'." >&2
echo "Get the correct access token from https://huggingface.co/settings/tokens and set if with 'hf auth login' " >&2
exit 1
fi

Problem: hf auth whoami returns the authenticated username, NOT a list of orgs/namespaces they have access to. This check will ALWAYS fail if HF_NAMESPACE is an organization name
(not the user's personal namespace).

Fix: Remove this check or query HF API properly to check org membership.

  1. Typo (line 505)

"set if with 'hf auth login'"
Should be: "set it with 'hf auth login'"

  1. Staging Directory Not Cleaned on Success (lines 524-526)

if [ -d "$CURRENT_STAGING_DIR_ABS" ]; then
rm -rf "$CURRENT_STAGING_DIR_ABS"
fi

This only removes the specific env's staging dir, not the parent hf-staging/ directory. After deploying 10 environments, you'll have hf-staging/ with 10 subdirectories.

Fix: Either clean up the parent or document it.

  1. Dockerfile Won't Work for Custom Environments

If someone creates src/envs/my_awesome_env/, the generated Dockerfile will be the generic template, which might not have the right dependencies.

Better approach: Check if src/envs/$ENV_NAME/server/Dockerfile exists, and if so, use it (like old script does).


Recommendation Summary

Immediate Action:

  1. Fix the authentication bug (lines 502-507) - it will block deployments to orgs
  2. Fix typo ("set if" → "set it")
  3. Decide on architectural approach (Option A vs B)

If Going with Option A (Merge):

Request the contributor to:

  1. Refactor to use existing Dockerfiles when available
  2. Add --no-push flag for CI/CD compatibility
  3. Update GitHub Actions workflow to use new script
  4. Deprecate old prepare_hf_deployment.sh

If Going with Option B (Keep Both):

  1. Merge as-is after fixing bugs
  2. Document clearly: "For community contributors only"
  3. Add note in old script: "For official CI/CD only"
  4. Accept maintenance overhead

Draft Feedback

Thanks for this contribution @burtenshaw! This is a valuable improvement that enables community contributors to deploy their environments to HF Spaces.

@pankit-eng raises a good question about reconciling with existing infrastructure. Here's my analysis:

Current Situation

We have three pieces:

  1. scripts/prepare_hf_deployment.sh - Staging only
  2. .github/workflows/deploy-hf-env.yml - CI/CD workflow (uses Initial skeleton #1 + git push)
  3. scripts/deploy_to_hf.sh (this PR) - End-to-end deployment with hf CLI

Recommendation: Merge and Refactor

I recommend we consolidate around this new script (it's better designed) but refactor it to work for both use cases:

Changes Needed:

  1. Use existing Dockerfiles when available (lines 160-283):

    • Check if src/envs/$ENV_NAME/server/Dockerfile exists
    • If yes, copy and modify it (like old script does)
    • If no, fall back to templates
    • This ensures community envs with custom Dockerfiles work automatically
  2. Fix authentication bug (lines 502-507):

    • hf auth whoami doesn't return org membership
    • This check will fail for organization namespaces
    • Either remove or query HF API properly
  3. Add --no-push flag:

    • Allows CI/CD workflow to call this script for staging
    • Then workflow can handle push separately if needed
  4. Fix typo (line 505): "set if with" → "set it with"

  5. Consider cleanup (lines 524-526):

    • Currently leaves hf-staging/ parent directory
    • Add option to clean entire staging dir?

Next Steps:

  1. Make these changes to deploy_to_hf.sh
  2. Update .github/workflows/deploy-hf-env.yml to use new script
  3. Deprecate prepare_hf_deployment.sh (or mark it for removal)

This gives us:

  • ✅ Single source of truth
  • ✅ Community contributors can deploy
  • ✅ CI/CD continues working
  • ✅ Reuses actual tested Dockerfiles

@burtenshaw
Copy link
Collaborator Author

burtenshaw commented Oct 31, 2025

This now makes the prepare_hf_deployment.sh script obsolete.

I've tested it by deploying a test env here with this run

note:

  • I've added parameters so that we can deploy custom envs, make envs private, deploy with suffixes, and deploy on namespaces other than openenv.
  • We need to update the HF_TOKEN secret for this repo. It does not have write permissions for the openenv org.

@zkwentz

@jspisak
Copy link
Contributor

jspisak commented Oct 31, 2025

@zkwentz - if you want to generate a new token i can update the secret

Copy link
Contributor

@zkwentz zkwentz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much cleaner, thank you @burtenshaw!

@zkwentz
Copy link
Contributor

zkwentz commented Nov 1, 2025

@zkwentz - if you want to generate a new token i can update the secret

DM'ed.

Note: I think the settings that Contributors can set and the settings that Repo Owners (@jspisak) can set are in conflict. I removed the one Contributors can set.

@zkwentz zkwentz merged commit a74cefa into meta-pytorch:main Nov 1, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants